-
Notifications
You must be signed in to change notification settings - Fork 703
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dual output recording #4794
base: master
Are you sure you want to change the base?
Dual output recording #4794
Conversation
BundleMonFiles updated (1)
Unchanged files (3)
Total files change +30.74KB +0.26% Final result: ✅ View report in BundleMon website ➡️ |
…rimary to vertical and record vertical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the changes requested are cosmetic. The missing translations are probably the most important. The RxJS behavior following after, but it seems to be working correctly, so definitely up to you whether to refactor would provide value.
<>REC</> | ||
)} | ||
</span> | ||
<span>{recordingStopping ? <i className="fa fa-spinner fa-pulse" /> : <>REC</>}</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Should REC
be translated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how it was originally but you bring up a good point. The limitation is that there is only room for three characters on the button. Let's bring it up to the team?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea the character limit is the issue here, as well as i'm not certain other languages have the same shorthand that we do for this term. i think looking into an icon replacement could have value here, as there is a generally accepted icon for recording
|
||
const { Option } = Select; | ||
|
||
const recordingQualities = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe these should be translated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
// Refactor when migrate to new API, sleep to allow state to update | ||
await new Promise(resolve => setTimeout(resolve, 300)); | ||
this.toggleRecording(); | ||
signalChanged.unsubscribe(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remember correctly RxJS didn't play nicely when unsubscribing to a subject inside a subscribe
function. I believe this could be achieved with take(1)
or takeWhile(condition)
and then subscribe
which would automatically unsubscribe for us.
Bear in mind is been a while so API might have changed or even this assumption.
this.signalInfoChanged.pipe(
take(1)
).subscribe(...)
I believe the filter (if takeWhile isn't needed after all, it might) and the delay could also be achieved with operators, but that's not a real concern, just the subscribe/unsubscribe behavior is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for tracking, discussed
// Refactor when move streaming to new API | ||
if (this.state.verticalStreamingStatus !== EStreamingState.Offline) { | ||
this.SET_VERTICAL_STREAMING_STATUS(EStreamingState.Offline); | ||
} | ||
signalChanged.unsubscribe(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RxJS unsubscribe comment earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed
} | ||
}, 2000), | ||
); | ||
recordingStopped.unsubscribe(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RxJS unsubscribe comment earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed
|
||
:global(.ant-form-item-label) { | ||
display: none; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not worth requiring a change here but just a lil useful thing-- we have a boolean property nowrap
on all of our react inputs that gets rid of the label portion
} | ||
|
||
return true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to follow the logic chain here, users can't stream and record to vertical simultaneously?
No description provided.